-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move notebook functions into .py file and add tests (which will fail) #3
base: master
Are you sure you want to change the base?
Conversation
|
||
python: | ||
- 3.6.0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add testing for Python 2.6, 2.7, 3.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docstring to the code
shifty.py
Outdated
@@ -7,7 +7,7 @@ | |||
import matplotlib.mlab as mlab | |||
from scipy.stats import norm | |||
# import BDdb | |||
import astrodb | |||
from astrodbkit import astrodb | |||
import pandas as pd | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring to the code
|
||
Arguments: y_array1, y_array2, unc1, unc2, n | ||
Outputs: avg, error, RMS""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By docstring instructions, add an example of the code being used. More detail can be added to the shapes/types of the individual inputs and outputs. Each can get a one line description.
RMS = [] | ||
|
||
for w in range(n): | ||
y_array1_rand = y_array1 + (np.random.randn(len(y_array1)) * unc1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line by line comments would be helpful, describing what the individual line achieves. For example, this line 'wiggles' within the uncertainty.
unc = np.asarray(unc_array) | ||
avg_unc = np.sum(unc) / len(unc) | ||
if avg_unc > 4.0: | ||
unc = 1.0 / unc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the relationship of SNR to uncertainty is unc=1/SNR? Add comment next to if statement making this clear - that an average above 4 indicates the array is SNR data
"select sources.id, sources.shortname, spectra.wavelength, spectra.flux, spectra.unc, radial_velocities.radial_velocity from sources join spectra on sources.id=spectra.source_id join radial_velocities on spectra.source_id=radial_velocities.source_id where spectra.source_id={} and spectra.wavelength_order={}".format( | ||
tar_source_id, spec_order)) | ||
|
||
w_tar = np.asarray(data_tar[0][2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment w is wavelength, f is frequency, etc.
Arguments: wavelength array, rv | ||
Outputs: shifted_wavelength""" | ||
|
||
shifted_wavelength = wavelength * (1. - (rv / 2.99792458e5)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is that number? Comment - also if it is a known constant try using astropy.constants, if the attached units give trouble, at end of variable put variable.value()
|
||
|
||
def normalize_spectra(f1, unc1, f2, unc2): | ||
"""Normalized one spectrum to another |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter which argument spectrum is being normalized to the other?
|
||
|
||
|
||
def mc_output_hist(MC_calculations, bins=25): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document this function
# yfcq_auto | ||
|
||
# The inputs are the target source id, the spec order being compared and the path to the text file with the comparison sample (has to be tab delimited) | ||
def yfcq(tar_source_id, spec_order, path_to_comp_sample_dataframe): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this function to docstring convention
"select sources.id, sources.shortname, spectra.wavelength, spectra.flux, spectra.unc, radial_velocities.radial_velocity from sources join spectra on sources.id=spectra.source_id join radial_velocities on spectra.source_id=radial_velocities.source_id where spectra.source_id={} and spectra.wavelength_order={}".format( | ||
tar_source_id, spec_order)) | ||
|
||
# Separates the target data into separate variables (wavelength, flux, RV, uncertainty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good description of variables - add to above functions
|
||
|
||
|
||
def vis_rms_results(tar_source_id, spec_order, path_to_sorted_dataframe): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well commented, but no docstrings.
Docstrings should include: |
No description provided.